Badge aria hidden removal #22858
Badge aria hidden removal #22858
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 6e1e995:
|
📊 Bundle size report
Unchanged fixtures
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 5ed22515a7542b432ed694f22606ac3718d501cb (build) |
| ```jsx | ||
| <button> | ||
| Inbox | ||
| <Badge>6<span class="visuallyHidden"> unread messages</span></Badge> |
There was a problem hiding this comment.
during Badge design review it was agreed to put additional information to the description, can you please elaborate on reasons for changing the recommendation?
There was a problem hiding this comment.
maybe something changed so it can be worth re-testing but I remember bugs where hidden texts were causing issues with voice control / dictation - as the voice control software required to read the whole label in order to navigate to the element. Hiding works very well for screen readers, but it might not be the case for all scenarios :(
There was a problem hiding this comment.
Did a quick test:
<html>
<head>
<style>
.visually-hidden {
clip: rect(0 0 0 0);
clip-path: inset(50%);
height: 1px;
overflow: hidden;
position: absolute;
white-space: nowrap;
width: 1px;
}
</style>
</head>
<body>
<button>Batman</button>
<button>Strawberry <span class="visually-hidden">superhero</span></button>
</body>I can use Voice Control to click on Batman, but Strawberry is not being recognized.
There was a problem hiding this comment.
@jurokapsiar which voice control software are you using? Partial labels have not been a problem in the past, at least with the voice control software I'm familiar with. If it is now, that seems like a major issue for a lot of common approaches to accessible text, and probably something we should follow up on as a bug with the software itself.
The issue with putting badge info as a description mostly falls into two categories:
- The description needs to go on something that accepts an accessible name/description. It's worth noting that this did not happen in a number of our existing examples, meaning the description was effectively worthless.
- A description is much less likely to be read, since it depends heavily on the user having their verbosity relatively high (or specifically turning on descriptions). Generally the accessible description should be used for information that is fully optional/unnecessary for the user to receive. Sometimes badges will fall into this category, hence the best practices text on how to mark those up. I just don't think it should be our default.
The first issue is the much more serious one. Based on the variety of places a badge can be used, the only real safe way to default to exposing the text is to have it show up as actual content.
| }, | ||
| root: getNativeElementProps('div', { | ||
| ref, | ||
| 'aria-hidden': true, |
There was a problem hiding this comment.
what is the impact of this change for Avatar? That is a role='img' with now a text inside of it. It would be good to test with screen readers and see if we need to give some best practices for Avatar usage.
There was a problem hiding this comment.
Avatar currently has aria-labelledby pointing to both itself and the badge, which right now has aria-hidden. Where that currently works, we're relying on behavior quirks. It'd be better to remove the aria-hidden so the accname for the image is built from non-hidden content.
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1267 | 1204 | 5000 | |
| Button | mount | 739 | 753 | 5000 | |
| FluentProvider | mount | 2124 | 2154 | 5000 | |
| FluentProviderWithTheme | mount | 352 | 385 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 336 | 344 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 420 | 460 | 10 | |
| MakeStyles | mount | 1889 | 2034 | 50000 |
remove aria-hidden from badge, and add docs
6 participants
Moving the removal of badge's aria-hidden into it's own PR rather than being a part of #22775